Skip to content

<fix>[core]: add @NoLogging to sensitive config fields#3342

Open
ZStack-Robot wants to merge 1 commit into5.5.6from
sync/ye.zou/fix/ZSTAC-80664
Open

<fix>[core]: add @NoLogging to sensitive config fields#3342
ZStack-Robot wants to merge 1 commit into5.5.6from
sync/ye.zou/fix/ZSTAC-80664

Conversation

@ZStack-Robot
Copy link
Collaborator

Resolves: ZSTAC-80664\n\nAdd @nologging annotation to prevent sensitive configuration data from being logged.

sync from gitlab !9171

@coderabbitai
Copy link

coderabbitai bot commented Feb 13, 2026

No actionable comments were generated in the recent review. 🎉


Walkthrough

在 ExternalPrimaryStorageInventory 构造器中新增对 config 的脱敏处理:调用 desensitizeConfig(config),并新增 desensitizeConfigdesensitizeUrlListdesensitizeUrl 三个私有方法,对 mdsUrlsmdsInfos 列表中 URL 的 @ 之后部分替换为 "***"

Changes

Cohort / File(s) Summary
脱敏逻辑
header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageInventory.java
添加 ArrayList/Map 导入;在 ExternalPrimaryStorageInventory(ExternalPrimaryStorageVO) 中对 config 调用 desensitizeConfig(config);新增私有方法 desensitizeConfig(Map), desensitizeUrlList(Map,String)desensitizeUrl(String),对 mdsUrlsmdsInfos 中的 URL 在 @ 后部分替换为 "***"

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

诗歌

🐰 我在配置里轻轻嗅,
把秘密用星星藏在后,
mds 的路徑被遮掩,
数据静静更温柔,
兔子在角落眨眨眼。


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 3 warnings)

Check name Status Explanation Resolution
Title check ❌ Error Pull request title follows the required [scope]: format with 54 characters, which is within the 72 character limit. However, the title mentions adding @NoLogging annotation while the actual changes implement a desensitizeConfig() method approach instead. Update the title to accurately reflect the implementation: '[core]: Desensitize sensitive configuration data in storage inventory' or similar, as the changes use URL masking methods rather than @NoLogging annotations.
Description check ⚠️ Warning The pull request description mentions adding @NoLogging annotation, but the actual implementation adds desensitizeConfig() and desensitizeUrl() methods to mask sensitive URLs. The description does not accurately represent the actual changes made. Update the description to clarify that the fix implements URL desensitization methods rather than @NoLogging annotations, explaining how the sensitive configuration data is masked during inventory initialization.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Merge Conflict Detection ⚠️ Warning ❌ Merge conflicts detected (2 files):

⚔️ compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java (content)
⚔️ header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageInventory.java (content)

These conflicts must be resolved before merging into 5.5.6.
Resolve conflicts locally and push changes to this branch.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sync/ye.zou/fix/ZSTAC-80664
⚔️ Resolve merge conflicts (beta)
  • Auto-commit resolved conflicts to branch sync/ye.zou/fix/ZSTAC-80664
  • Create stacked PR with resolved conflicts
  • Post resolved changes as copyable diffs in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@MatheMatrix MatheMatrix force-pushed the sync/ye.zou/fix/ZSTAC-80664 branch from 6902789 to 8a796dc Compare February 13, 2026 11:40
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In
`@header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageInventory.java`:
- Around line 91-95: The desensitizeUrl method currently strips the protocol
(e.g., turning "iscsi://user:pass@host" into "***@host"); update desensitizeUrl
so it preserves the scheme/protocol prefix (everything up to and including
"://") and only masks the credentials portion before the '@' (replace credential
characters with "***" or similar) while keeping the rest of the URL intact;
locate and modify the desensitizeUrl method to detect the "://" delimiter and
the last '@' (using the existing atIndex logic) and build the returned string as
protocol + maskedCredentials + substring(from atIndex) so callers depending on
protocol parsing keep backward-compatible behavior.

The mdsUrls field in ExternalPrimaryStorage config contains
user:password@host format credentials. Add desensitization to
mask credentials as ***@host in API/CLI output.

Resolves: ZSTAC-80664

Change-Id: I94bdede5a1b52eb039de70efb5458693484405f7
@MatheMatrix MatheMatrix force-pushed the sync/ye.zou/fix/ZSTAC-80664 branch from 8a796dc to aaeaf39 Compare February 16, 2026 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants